Decoupling Matrix creation from Application by ouchadam · Pull Request #5185 · element-hq/element-android

ouchadam · 2022-02-08T15:45:45Z

Decouples the creation of our Matrix instance from the application by using the DI factory instead.
This has the benefit of allowing the MatrixConfiguration to use injectable dependencies.

Deprecates Matrix.getInstance, we're now advocating for clients to create and maintain their own singletons instead, instances of Matrix can be created with Matrix.createInstance

…low us to inject a consistent instance

allows the matrix configuration to also contain dependencies

…uld also be backed back the test module instead of the singleton state

…y calling getInstance

ouchadam · 2022-02-08T15:46:17Z

vector/src/main/java/im/vector/app/core/di/SingletonModule.kt


    @Provides
-    fun providesMatrix(context: Context): Matrix {
+    fun providesMatrixConfiguration(context: Context): MatrixConfiguration {


extra dependencies can be added here, such as the VectorFeatures!

github-actions · 2022-02-08T16:55:17Z

Unit Test Results

  76 files ±0   76 suites ±0 52s ⏱️ -6s
144 tests ±0 144 ✔️ ±0 0 💤 ±0 0 ❌ ±0
452 runs ±0 452 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 5b851f1. ± Comparison against base commit 49a0555.

♻️ This comment has been updated with latest results.

ouchadam · 2022-02-08T17:20:10Z

vector/src/main/java/im/vector/app/VectorApplication.kt

        }
    }

-    override fun providesMatrixConfiguration(): MatrixConfiguration {


the main downside to using MatrixConfiguration.Provider is that it occurs during super.onCreate which means the dependencies declared in the application haven't been injected yet, making the object creation lifecycle a bit misleading

github-actions · 2022-02-08T17:23:24Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed=
[org.matrix.android.sdk.account]
passed=
[org.matrix.android.sdk.internal]
passed=
[org.matrix.android.sdk.ordering]
passed=
[org.matrix.android.sdk.PermalinkParserTest]
passed=

bmarty

Some quick feedback, would be nice to have @ganfra POV too.

bmarty · 2022-02-14T10:27:15Z

vector/src/main/java/im/vector/app/core/di/SingletonModule.kt

+    fun providesMatrixConfiguration(context: Context): MatrixConfiguration {
+        return MatrixConfiguration(
+                applicationFlavor = BuildConfig.FLAVOR_DESCRIPTION,
+                roomDisplayNameFallbackProvider = VectorRoomDisplayNameFallbackProvider(context)


We could probably also provide RoomDisplayNameFallbackProvider as a param.

will do! 👍

bmarty · 2022-02-14T10:29:00Z

vector/src/main/java/im/vector/app/VectorApplication.kt

 @HiltAndroidApp
 class VectorApplication :
        Application(),
-        MatrixConfiguration.Provider,


Maybe delete this interface? Or can it still be used by other application?

it could be used by other clients of the SDK, if we agree that the Matrix singleton API should avoid querying the application then we could mark the interface as deprecated and schedule removing it? potentially with some migration steps~

what do you think?

I defer to @ganfra :)

Yes I think we should remove this and the inner singleton to avoid mistakes!
Regarding the process, we could indeed mark them as deprecated in a first place.

added deprecations 95df3e7

bmarty · 2022-02-14T10:31:36Z

vector/src/main/java/im/vector/app/core/di/SingletonModule.kt

+    @Singleton
+    fun providesMatrix(context: Context, configuration: MatrixConfiguration): Matrix {
+        Matrix.initialize(context, configuration)
        return Matrix.getInstance(context)


Will this work since the context is not implementing MatrixConfiguration.Provider anymore?

https://github.com/vector-im/element-android/blob/main/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Matrix.kt#L108

Ah, just read again the description, sorry

no worries! yep it'll work but... it's only because of the exact order of these calls

initialise() // sets the private singleton instance getInstance() // the instance is not null so we avoid attempting to read from (application as Provider). providesMatrixConfiguration()

I could add another documented function to the companion object to only create an instance without making use of the internal singleton (in our case dagger/hilt will handle the singleton instance for us)

/** * Creates an instance of Matrix, it's recommend to manage this instance as a singleton. * To make use of the built in singleton use Matrix.initialise() or Matrix.getInstance(context) instead **/ fun createInstance(context: Context, matrixConfiguration: MatrixConfiguration) : Matrix { return Matrix(context.applicationContext, matrixConfiguration) }

I did it 😄 having the separate method really helps clarify the ambiguity fd2d928

Matrix.initialize and Matrix.getInstance are now unused in Element

…non singleton/duplicated singleton usages - also documents the static methods

bmarty

Thanks for the update. More comments. I will ping @ganfra to let him check the changes.

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Matrix.kt

bmarty · 2022-02-14T13:34:07Z

vector/src/main/java/im/vector/app/VectorApplication.kt

 @HiltAndroidApp
 class VectorApplication :
        Application(),
-        MatrixConfiguration.Provider,


I defer to @ganfra :)

bmarty · 2022-02-14T13:35:07Z

vector/src/main/java/im/vector/app/features/rageshake/BugReporter.kt

        private val vectorFileLogger: VectorFileLogger,
-        private val systemLocaleProvider: SystemLocaleProvider
+        private val systemLocaleProvider: SystemLocaleProvider,
+        private val matrix: Matrix


I like that!

bmarty · 2022-02-14T13:36:27Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Matrix.kt

+        /**
+         * Either provides an already initialized singleton Matrix instance or queries the application context for a MatrixConfiguration.Provider
+         * to lazily create and store the instance.
+         */


Maybe update the comment at line 132 ("You should call Matrix.initialize or let your application implements MatrixConfiguration.Provider.") to inform developers that they can manage the singleton themselves by using createInstance

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Matrix.kt

…trix.kt Co-authored-by: Benoit Marty <benoitm@matrix.org>

… in favour of clients controlling their own instances

…tor-im/element-android into feature/adm/decouple-matrix-creation

bmarty

Thanks for the update and thanks @ganfra for your review!

ouchadam added 4 commits February 8, 2022 14:02

avoiding using the matrix getInstance internally within the sdk to al…

2d80c6b

…low us to inject a consistent instance

always providing matrix via the singleton module -

c160f5d

allows the matrix configuration to also contain dependencies

initialising a new matrix instance for android tests, ideally this wo…

a63e79b

…uld also be backed back the test module instead of the singleton state

using injected matrix instance for the bug reporting to avoid directl…

8dc8c20

…y calling getInstance

ouchadam commented Feb 8, 2022

View reviewed changes

removing unused import

0390994

ouchadam closed this Feb 8, 2022

marking the matrix provision as a singleton to avoid reinitialising

5ff420f

ouchadam reopened this Feb 8, 2022

adding changelog entry

c62b994

ouchadam commented Feb 8, 2022

View reviewed changes

bmarty reviewed Feb 14, 2022

View reviewed changes

bmarty requested a review from ganfra February 14, 2022 10:32

ouchadam added 2 commits February 14, 2022 12:30

injecting the room name provider as we're within hilts scope

674aea9

creating separate creator for the matrix instance to avoid ambiguous …

fd2d928

…non singleton/duplicated singleton usages - also documents the static methods

bmarty approved these changes Feb 14, 2022

View reviewed changes

ouchadam and others added 6 commits February 14, 2022 13:41

Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Ma…

2f7f86b

…trix.kt Co-authored-by: Benoit Marty <benoitm@matrix.org>

Update matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/Ma…

2eb417a

…trix.kt Co-authored-by: Benoit Marty <benoitm@matrix.org>

providing more alternatives when Matrix.getInstance fails

d05af1c

deprecating the Matrix.initialize and Matrix.getInstance entry points…

95df3e7

… in favour of clients controlling their own instances

Merge branch 'feature/adm/decouple-matrix-creation' of github.com:vec…

8e0f580

…tor-im/element-android into feature/adm/decouple-matrix-creation

suppressing deprecated warning on an unused method

5b851f1

bmarty removed the request for review from ganfra February 17, 2022 14:34

bmarty approved these changes Feb 17, 2022

View reviewed changes

bmarty merged commit ac9f138 into develop Feb 17, 2022

bmarty deleted the feature/adm/decouple-matrix-creation branch February 17, 2022 14:35

Conversation

ouchadam commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Matrix SDK

Integration Tests Results:

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouchadam Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ouchadam commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading

ouchadam Feb 14, 2022 •

edited

Loading

ouchadam Feb 14, 2022 •

edited

Loading

ouchadam Feb 14, 2022 •

edited

Loading